Skip to content

Change ParamaterDecorator to allow an undefined propertyKey #53365

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 20, 2023

Conversation

dhritzkiv
Copy link
Contributor

@dhritzkiv dhritzkiv commented Mar 20, 2023

propertyKey argument can be undefined for constructor parameter decorators.

I believe changes between 4.9 and 5.0 tightened up decorator type checking in some areas (#52435), while a newly introduced declaration file for legacy decorators doesn't reflect this change.

After upgrading to 5.0, I noticed that in my code, a dependency's usage of ParameterDecorator resulted in a type checking warning:

Unable to resolve signature of parameter decorator when called as an expression.
  Argument of type 'undefined' is not assignable to parameter of type 'string | symbol'

Changing the propertyKey in decorators.legacy.d.ts to allow undefined fixed the type-checking error.

I did not touch the other Decorator types as I don't know if the propertyKey issue affects them in the same way.

Fixes #53312

`propertyKey` argument can be undefined for parameter decorators
@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Mar 20, 2023
@jakebailey jakebailey closed this Mar 20, 2023
@jakebailey jakebailey reopened this Mar 20, 2023
@jakebailey
Copy link
Member

(ignore the close/open; just fixing CI after I broke/unbroke main 🙃)

@dhritzkiv
Copy link
Contributor Author

OK! I was worried I did something wrong to have the tests break like that. 😌

@dhritzkiv dhritzkiv marked this pull request as ready for review March 20, 2023 04:59
@rbuckton rbuckton merged commit 41474f9 into microsoft:main Mar 20, 2023
@jakebailey
Copy link
Member

jakebailey commented Mar 20, 2023

This needs a backport to 5.0, right? @DanielRosenwasser

@rbuckton
Copy link
Member

@DanielRosenwasser: we may want to include this if we do any follow-up 5.0.x patches. It is a potentially breaking change, but is in line with the correctness change we made for parameter decorators that we mention in the 5.0 release notes.

@jakebailey
Copy link
Member

@typescript-bot cherry-pick this to release-5.0

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 20, 2023

Heya @jakebailey, I've started to run the task to cherry-pick this into release-5.0 on this PR at 2c400b7. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, I've opened #53392 for you.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Mar 20, 2023
Component commits:
2c400b7 Update decorators.legacy.d.ts
`propertyKey` argument can be undefined for parameter decorators
@dhritzkiv dhritzkiv deleted the patch-2 branch March 20, 2023 19:14
@DanielRosenwasser
Copy link
Member

Technically any library that exports a `ParameterDecorator is potentially "wrong" today; but on the other hand, any existing code is probably sufficiently working anyway. So I think a cherry-pick is reasonable because it should unblock consumers, while helping library authors adapt.

DanielRosenwasser pushed a commit that referenced this pull request Mar 27, 2023
drivron pushed a commit to scenari/typescript that referenced this pull request Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Legacy/Experimental parameter decorators fails with TS1239 when StrictNullChecks is true in Typescript 5.0.2
5 participants